Allow FetchSubPhaseProcessors to report their required stored fields …#1
Allow FetchSubPhaseProcessors to report their required stored fields …#1MitchLewis930 wants to merge 1 commit intopr_011_beforefrom
Conversation
…lastic#91269) Loading of stored fields is currently handled directly in FetchPhase, with some fairly complex logic examining various bits of the FetchContext to work out what fields need to be loaded. This is further complicated by synthetic source, which may have its own stored field requirements. This commit tries to separate out these concerns a little by adding a new StoredFieldsSpec record that holds information about which stored fields need to be loaded. Each FetchSubPhaseProcessor can now report a StoredFieldsSpec detailing what its requirements are, and these specs can be merged together, along with requirements from a SourceLoader, to determine up-front what fields should be loaded by the StoredFieldLoader. The stored fields themselves are added into the SearchHit by a new StoredFieldsPhase, which handles alias resolution and value post- processing. The logic to determine when source should be loaded and when not, based on the presence of script fields or stored fields, is moved into FetchContext, which highlights some inconsistencies that can be fixed in follow-up commits.
📝 WalkthroughWalkthroughThis pull request refactors Elasticsearch's fetch and stored fields handling to improve modularity and clarity. Key changes include introducing a new Changes
Sequence Diagram(s)The changes do not meet the criteria for sequence diagram generation as they primarily involve:
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/parent-join/src/yamlRestTest/resources/rest-api-spec/test/30_inner_hits.yml (1)
146-154: Duplicate assertions detected in profile breakdown checks.Lines 148-149 duplicate the assertions from lines 146-147, and lines 153-154 duplicate lines 151-152. This appears to be a copy-paste error that should be cleaned up.
🛠️ Suggested fix
- match: { profile.shards.0.fetch.children.0.type: FetchSourcePhase } - gt: { profile.shards.0.fetch.children.0.breakdown.next_reader_count: 0 } - gt: { profile.shards.0.fetch.children.0.breakdown.next_reader: 0 } - - gt: { profile.shards.0.fetch.children.0.breakdown.next_reader_count: 0 } - - gt: { profile.shards.0.fetch.children.0.breakdown.next_reader: 0 } - match: { profile.shards.0.fetch.children.1.type: InnerHitsPhase } - gt: { profile.shards.0.fetch.children.1.breakdown.next_reader_count: 0 } - gt: { profile.shards.0.fetch.children.1.breakdown.next_reader: 0 } - - gt: { profile.shards.0.fetch.children.1.breakdown.next_reader_count: 0 } - - gt: { profile.shards.0.fetch.children.1.breakdown.next_reader: 0 } - match: { profile.shards.0.fetch.children.2.type: StoredFieldsPhase }
🤖 Fix all issues with AI agents
In `@server/src/main/java/org/elasticsearch/search/fetch/FetchContext.java`:
- Around line 67-73: The loop over sfc.fieldNames() can NPE when fieldNames()
returns null (e.g., metadataOnly()); before iterating, guard with a null check
on sfc.fieldNames() — in the block that checks sfc != null && sfc.fetchFields(),
add a condition or early continue ensuring sfc.fieldNames() != null, then
perform the for-each and update fsc using FetchSourceContext.of(true) when
SourceFieldMapper.NAME matches.
In
`@server/src/main/java/org/elasticsearch/search/fetch/FetchSubPhaseProcessor.java`:
- Around line 40-43: The new abstract method storedFieldsSpec() on
FetchSubPhaseProcessor will break binary compatibility for existing plugins;
change its declaration to a default method on the FetchSubPhaseProcessor
interface that returns StoredFieldsSpec.NO_REQUIREMENTS so existing
implementations continue to work without recompilation, i.e., update the
storedFieldsSpec() signature in FetchSubPhaseProcessor to a default
implementation returning StoredFieldsSpec.NO_REQUIREMENTS.
In `@server/src/main/java/org/elasticsearch/search/SearchHit.java`:
- Around line 433-436: The public method SearchHit.addDocumentFields currently
calls putAll directly and will NPE if callers pass null; update
addDocumentFields to guard against null inputs by checking docFields and
metaFields before calling this.documentFields.putAll(docFields) and
this.metaFields.putAll(metaFields) (treat null as no-op/empty), so the method
tolerates null maps and preserves previous constructor behavior.
In `@server/src/test/java/org/elasticsearch/search/SearchHitTests.java`:
- Around line 396-399: The test creates a metadata Map<String, DocumentField>
containing a "_routing" DocumentField but then calls
SearchHit.addDocumentFields(fields, Collections.emptyMap()), leaving the
metadata unused; update the call to pass the created metadata map so the test
exercises metadata handling (i.e., replace the empty map argument with the
metadata variable) and verify SearchHit serialization/deserialization or
assertions that rely on the "_routing" metadata field (locate the SearchHit
instantiation, the metadata Map declaration, and the addDocumentFields(...)
call).
In
`@x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/persistence/MockClientBuilder.java`:
- Around line 192-193: The call to SearchHit.addDocumentFields has its arguments
reversed: pass the test document fields as the first argument and the empty
metadata map as the second; update the call in MockClientBuilder where SearchHit
hit = new SearchHit(10, null) is created to call addDocumentFields with
fields.get(i) as the first parameter and Map.of() as the second (i.e., correct
the argument order for addDocumentFields(Map<String, DocumentField> docFields,
Map<String, DocumentField> metaFields)).
🧹 Nitpick comments (9)
server/src/main/java/org/elasticsearch/search/DefaultSearchContext.java (1)
405-408: LGTM! Correct semantic tightening ofhasScriptFields().The updated logic correctly returns
trueonly when script fields actually exist, rather than when the context object is merely non-null. This aligns with the lazy initialization inscriptFields()which creates an empty context on first access.Optional style nit: Consider using the more idiomatic negation:
return scriptFields != null && !scriptFields.fields().isEmpty();,
server/src/main/java/org/elasticsearch/index/fieldvisitor/CustomFieldsVisitor.java (1)
26-30: Consider usingremoveAllfor a more idiomatic approach.The defensive copy and metadata removal logic is correct. A minor improvement would be to use
removeAllinstead of creating a temporaryListand iterating:♻️ Suggested refactor
public CustomFieldsVisitor(Set<String> fields, boolean loadSource) { super(loadSource); this.fields = new HashSet<>(fields); // metadata fields are already handled by FieldsVisitor, so removing // them here means that if the only fields requested are metadata // fields then we can shortcut loading - List.of("_id", "_routing", "_source").forEach(this.fields::remove); + this.fields.removeAll(Set.of("_id", "_routing", "_source")); }This also allows removing the
java.util.Listimport on line 13 if not used elsewhere.server/src/main/java/org/elasticsearch/search/fetch/StoredFieldsSpec.java (2)
28-33: Addfinalmodifier to static constants.Public static fields should be declared
finalto prevent accidental reassignment.♻️ Suggested fix
- public static StoredFieldsSpec NO_REQUIREMENTS = new StoredFieldsSpec(false, false, Set.of()); + public static final StoredFieldsSpec NO_REQUIREMENTS = new StoredFieldsSpec(false, false, Set.of()); /** * Use when the source should be loaded but no other stored fields are required */ - public static StoredFieldsSpec NEEDS_SOURCE = new StoredFieldsSpec(true, false, Set.of()); + public static final StoredFieldsSpec NEEDS_SOURCE = new StoredFieldsSpec(true, false, Set.of());
14-18: Javadoc missing@paramforrequiresMetadata.The record parameter
requiresMetadatais not documented.📝 Suggested fix
/** * Defines which stored fields need to be loaded during a fetch * `@param` requiresSource should source be loaded + * `@param` requiresMetadata should metadata fields be loaded * `@param` requiredStoredFields a set of stored fields to load */server/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/HighlightPhase.java (1)
117-167: Avoid collecting stored field names that are never used.
storedFieldsis populated but the merged spec always usesSet.of(), so the set is dead work right now. If highlighting shouldn't request stored fields yet, consider dropping the set (andHashSetimport); otherwise, wire it intoStoredFieldsSpecwhen ready.♻️ Possible simplification
- Set<String> storedFields = new HashSet<>(); boolean sourceRequired = forceSource; for (String fieldName : fieldNamesToHighlight) { MappedFieldType fieldType = context.getSearchExecutionContext().getFieldType(fieldName); @@ - if (fieldType.isStored()) { - storedFields.add(fieldType.name()); - } else { + if (fieldType.isStored() == false) { sourceRequired = true; }server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.java (1)
43-47: Consider narrowing stored-fields requirements to avoid unnecessary _source loads.
IfFieldFetchercan determine which fields truly need source vs stored/docvalues, deriving a more preciseStoredFieldsSpecwould reduce IO. The TODO hints at this; consider tracking it as a follow-up.server/src/main/java/org/elasticsearch/search/fetch/subphase/InnerHitsPhase.java (1)
67-75: Consider early return for minor optimization.The current implementation evaluates all conditions for every subcontext even after
requiresSourcebecomestrue. For most use cases this is fine, but an early return could skip unnecessary iterations.♻️ Optional: Early return optimization
private static boolean requiresSource(Collection<? extends SearchContext> subContexts) { - boolean requiresSource = false; for (SearchContext sc : subContexts) { - requiresSource |= sc.sourceRequested(); - requiresSource |= sc.fetchFieldsContext() != null; - requiresSource |= sc.highlight() != null; + if (sc.sourceRequested() || sc.fetchFieldsContext() != null || sc.highlight() != null) { + return true; + } } - return requiresSource; + return false; }server/src/main/java/org/elasticsearch/search/fetch/FetchSubPhase.java (1)
31-38: Consider immutability for theloadedFieldsmap.The
loadedFieldsmap is stored and returned directly without wrapping in an unmodifiable view. If callers can modify the map after construction, this could lead to subtle bugs. Consider wrapping inCollections.unmodifiableMap()orMap.copyOf()if the map should be immutable after construction.However, if performance is critical and callers are trusted internal code, the current approach is acceptable.
♻️ Optional: wrap in unmodifiable view
public HitContext(SearchHit hit, LeafReaderContext context, int docId, Map<String, List<Object>> loadedFields, Source source) { this.hit = hit; this.readerContext = context; this.docId = docId; this.source = source; - this.loadedFields = loadedFields; + this.loadedFields = loadedFields.isEmpty() ? Map.of() : Collections.unmodifiableMap(loadedFields); }server/src/test/java/org/elasticsearch/search/fetch/StoredFieldsSpecTests.java (1)
73-78: Note: ScriptField constructed with null script.The
ScriptFieldsContext.ScriptFieldis created with anullscript (Line 76). This works for the current test sinceScriptFieldsPhase.getProcessor()likely only checks for the presence of script fields, not their actual scripts. However, this coupling could break if the implementation changes.Consider adding a comment explaining why
nullis acceptable here, or use a minimal valid script if easily available.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (84)
docs/reference/search/profile.asciidocmodules/lang-mustache/src/test/java/org/elasticsearch/script/mustache/SearchTemplateResponseTests.javamodules/parent-join/build.gradlemodules/parent-join/src/yamlRestTest/resources/rest-api-spec/test/30_inner_hits.ymlmodules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorHighlightSubFetchPhase.javamodules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorMatchedSlotSubFetchPhase.javamodules/percolator/src/test/java/org/elasticsearch/percolator/PercolatorMatchedSlotSubFetchPhaseTests.javamodules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/DiscountedCumulativeGainTests.javamodules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/ExpectedReciprocalRankTests.javamodules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/MeanReciprocalRankTests.javamodules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/PrecisionAtKTests.javamodules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/RankEvalResponseTests.javamodules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/RatedSearchHitTests.javamodules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/RecallAtKTests.javamodules/reindex/src/test/java/org/elasticsearch/reindex/AsyncBulkByScrollActionTests.javamodules/reindex/src/test/java/org/elasticsearch/reindex/ClientScrollableHitSourceTests.javarest-api-spec/build.gradlerest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/370_profile.ymlserver/src/internalClusterTest/java/org/elasticsearch/action/search/TransportSearchIT.javaserver/src/internalClusterTest/java/org/elasticsearch/search/fetch/FetchSubPhasePluginIT.javaserver/src/main/java/org/elasticsearch/index/fieldvisitor/CustomFieldsVisitor.javaserver/src/main/java/org/elasticsearch/index/get/ShardGetService.javaserver/src/main/java/org/elasticsearch/index/mapper/IgnoredFieldMapper.javaserver/src/main/java/org/elasticsearch/index/mapper/LegacyTypeFieldMapper.javaserver/src/main/java/org/elasticsearch/index/mapper/RoutingFieldMapper.javaserver/src/main/java/org/elasticsearch/search/DefaultSearchContext.javaserver/src/main/java/org/elasticsearch/search/SearchHit.javaserver/src/main/java/org/elasticsearch/search/SearchModule.javaserver/src/main/java/org/elasticsearch/search/fetch/FetchContext.javaserver/src/main/java/org/elasticsearch/search/fetch/FetchPhase.javaserver/src/main/java/org/elasticsearch/search/fetch/FetchProfiler.javaserver/src/main/java/org/elasticsearch/search/fetch/FetchSubPhase.javaserver/src/main/java/org/elasticsearch/search/fetch/FetchSubPhaseProcessor.javaserver/src/main/java/org/elasticsearch/search/fetch/StoredFieldsContext.javaserver/src/main/java/org/elasticsearch/search/fetch/StoredFieldsSpec.javaserver/src/main/java/org/elasticsearch/search/fetch/subphase/ExplainPhase.javaserver/src/main/java/org/elasticsearch/search/fetch/subphase/FetchDocValuesPhase.javaserver/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.javaserver/src/main/java/org/elasticsearch/search/fetch/subphase/FetchScorePhase.javaserver/src/main/java/org/elasticsearch/search/fetch/subphase/FetchSourcePhase.javaserver/src/main/java/org/elasticsearch/search/fetch/subphase/FetchVersionPhase.javaserver/src/main/java/org/elasticsearch/search/fetch/subphase/InnerHitsPhase.javaserver/src/main/java/org/elasticsearch/search/fetch/subphase/MatchedQueriesPhase.javaserver/src/main/java/org/elasticsearch/search/fetch/subphase/ScriptFieldsContext.javaserver/src/main/java/org/elasticsearch/search/fetch/subphase/ScriptFieldsPhase.javaserver/src/main/java/org/elasticsearch/search/fetch/subphase/SeqNoPrimaryTermPhase.javaserver/src/main/java/org/elasticsearch/search/fetch/subphase/StoredFieldsPhase.javaserver/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/HighlightPhase.javaserver/src/main/java/org/elasticsearch/search/internal/SubSearchContext.javaserver/src/test/java/org/elasticsearch/action/search/ExpandSearchPhaseTests.javaserver/src/test/java/org/elasticsearch/action/search/SearchPhaseControllerTests.javaserver/src/test/java/org/elasticsearch/action/search/SearchResponseTests.javaserver/src/test/java/org/elasticsearch/index/mapper/IgnoredFieldTypeTests.javaserver/src/test/java/org/elasticsearch/index/mapper/RoutingFieldTypeTests.javaserver/src/test/java/org/elasticsearch/search/SearchHitTests.javaserver/src/test/java/org/elasticsearch/search/SearchHitsTests.javaserver/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalTopHitsTests.javaserver/src/test/java/org/elasticsearch/search/fetch/StoredFieldsSpecTests.javaserver/src/test/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhaseTests.javaserver/src/test/java/org/elasticsearch/search/fetch/subphase/FetchSourcePhaseTests.javatest/framework/src/main/java/org/elasticsearch/search/fetch/HighlighterTestCase.javax-pack/plugin/async-search/src/test/java/org/elasticsearch/xpack/search/AsyncSearchSingleNodeTests.javax-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/action/EnrichShardMultiSearchAction.javax-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/execution/assembler/ImplicitTiebreakerTests.javax-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/execution/assembler/SequenceSpecTests.javax-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/execution/search/CriterionOrdinalExtractionTests.javax-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/execution/sequence/CircuitBreakerTests.javax-pack/plugin/logstash/src/test/java/org/elasticsearch/xpack/logstash/action/TransportGetPipelineActionTests.javax-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/action/TransportDeleteForecastActionTests.javax-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/datafeed/extractor/scroll/ScrollDataExtractorTests.javax-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/dataframe/process/DataFrameRowsJoinerTests.javax-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/persistence/JobResultsProviderTests.javax-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/persistence/MockClientBuilder.javax-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/test/SearchHitBuilder.javax-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/saml/TransportSamlInvalidateSessionActionTests.javax-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/TokenServiceTests.javax-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/service/IndexServiceAccountTokenStoreTests.javax-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.javax-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/extractor/ComputingExtractorTests.javax-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractorTests.javax-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/extractor/TopHitsAggExtractorTests.javax-pack/plugin/watcher/src/internalClusterTest/java/org/elasticsearch/xpack/watcher/condition/CompareConditionSearchTests.javax-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/WatcherServiceTests.javax-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/execution/TriggeredWatchStoreTests.java
🧰 Additional context used
🧬 Code graph analysis (9)
server/src/main/java/org/elasticsearch/search/fetch/FetchContext.java (3)
server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java (1)
SourceFieldMapper(34-273)server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchSourceContext.java (1)
FetchSourceContext(35-264)server/src/main/java/org/elasticsearch/search/fetch/StoredFieldsContext.java (1)
StoredFieldsContext(30-196)
server/src/main/java/org/elasticsearch/search/fetch/subphase/InnerHitsPhase.java (1)
server/src/main/java/org/elasticsearch/search/internal/SearchContext.java (1)
SearchContext(59-377)
server/src/main/java/org/elasticsearch/search/fetch/subphase/StoredFieldsPhase.java (7)
server/src/main/java/org/elasticsearch/index/mapper/IgnoredFieldMapper.java (1)
IgnoredFieldMapper(23-91)server/src/main/java/org/elasticsearch/index/mapper/LegacyTypeFieldMapper.java (1)
LegacyTypeFieldMapper(26-109)server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java (1)
MappedFieldType(58-606)server/src/main/java/org/elasticsearch/index/mapper/RoutingFieldMapper.java (1)
RoutingFieldMapper(21-134)server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java (1)
SourceFieldMapper(34-273)server/src/main/java/org/elasticsearch/search/fetch/FetchContext.java (1)
FetchContext(37-278)server/src/main/java/org/elasticsearch/search/fetch/StoredFieldsContext.java (1)
StoredFieldsContext(30-196)
server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java (1)
server/src/main/java/org/elasticsearch/index/fieldvisitor/StoredFieldLoader.java (1)
StoredFieldLoader(27-173)
server/src/main/java/org/elasticsearch/search/SearchModule.java (1)
server/src/main/java/org/elasticsearch/search/fetch/subphase/StoredFieldsPhase.java (1)
StoredFieldsPhase(36-122)
server/src/test/java/org/elasticsearch/search/fetch/StoredFieldsSpecTests.java (5)
server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java (1)
SearchSourceBuilder(73-1853)server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchSourcePhase.java (1)
FetchSourcePhase(22-97)server/src/main/java/org/elasticsearch/search/fetch/subphase/ScriptFieldsPhase.java (1)
ScriptFieldsPhase(26-95)server/src/main/java/org/elasticsearch/search/fetch/subphase/StoredFieldsPhase.java (1)
StoredFieldsPhase(36-122)server/src/main/java/org/elasticsearch/search/internal/SearchContext.java (1)
SearchContext(59-377)
server/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/HighlightPhase.java (1)
server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldFetcher.java (1)
FieldContext(294-302)
server/src/test/java/org/elasticsearch/index/mapper/IgnoredFieldTypeTests.java (1)
server/src/main/java/org/elasticsearch/index/mapper/IgnoredFieldMapper.java (1)
IgnoredFieldMapper(23-91)
server/src/test/java/org/elasticsearch/index/mapper/RoutingFieldTypeTests.java (1)
server/src/main/java/org/elasticsearch/index/mapper/RoutingFieldMapper.java (1)
RoutingFieldMapper(21-134)
🔇 Additional comments (117)
x-pack/plugin/watcher/src/internalClusterTest/java/org/elasticsearch/xpack/watcher/condition/CompareConditionSearchTests.java (1)
89-91: Constructor update matches new SearchHit API.
This is consistent with the refactor and keeps the test focused on score/shard fields.modules/lang-mustache/src/test/java/org/elasticsearch/script/mustache/SearchTemplateResponseTests.java (1)
160-160: Constructor update looks correct.The two-argument
SearchHitconstructor is sufficient for this test since only_idand_scoreare asserted.x-pack/plugin/logstash/src/test/java/org/elasticsearch/xpack/logstash/action/TransportGetPipelineActionTests.java (1)
213-223: Looks good with the newSearchHitconstructor.The updated instantiation fits the new API shape and the test only relies on id/score/shard.
x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/execution/sequence/CircuitBreakerTests.java (4)
109-113: LGTM — constructor update is consistent.
125-126: LGTM — new constructor fits test usage.
389-393: LGTM — constructor update aligns with new SearchHit API.
436-440: LGTM — constructor update aligns with new SearchHit API.x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/test/SearchHitBuilder.java (2)
24-25: Constructor update aligns with new SearchHit API.
32-34: Field attachment viasetDocumentFieldlooks correct.x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/execution/assembler/ImplicitTiebreakerTests.java (2)
76-82: LGTM!The constructor simplification from four arguments to two arguments is correct. The
sortValuesare properly attached post-construction, which aligns with the new pattern of using the simpler constructor and adding fields via dedicated API calls.
94-96: LGTM!The simplified two-argument
SearchHitconstructor is correctly used here. Since this is a fetch operation that only needs the hit reference ID, no additional fields are required.test/framework/src/main/java/org/elasticsearch/search/fetch/HighlighterTestCase.java (1)
65-70: LGTM — updated HitContext construction matches the new API.The test adapts cleanly to the new
SearchHitconstructor and loaded-fields map parameter.modules/reindex/src/test/java/org/elasticsearch/reindex/ClientScrollableHitSourceTests.java (1)
162-162: LGTM!The simplified
SearchHitconstructor usage aligns with the PR's refactoring to use the two-argument constructor. The chainedsourceRef()call properly sets the source content needed for the test assertions inassertSameHits.x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/persistence/JobResultsProviderTests.java (1)
924-926: LGTM!The migration to the new
SearchHitconstruction pattern is correctly implemented. The helper method now:
- Creates a
SearchHitwith the simplified two-argument constructor- Attaches document fields via
addDocumentFields- Sets the source via
sourceRefThis maintains functional equivalence with the previous approach while aligning with the broader refactoring in this PR.
x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/extractor/ComputingExtractorTests.java (1)
86-88: LGTM!The test correctly adapts to the new
SearchHitconstructor pattern by using the minimal constructor and then attaching the field viasetDocumentField. The test logic is preserved and aligns with the PR's goal of simplifyingSearchHitconstruction.x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/execution/assembler/SequenceSpecTests.java (2)
191-193: LGTM!The migration to the 2-argument
SearchHitconstructor with post-construction field attachment viaaddDocumentFields(documentFields, Map.of())is correct and aligns with the updated API pattern.
234-234: LGTM!Correct migration to the 2-argument
SearchHitconstructor. NoaddDocumentFieldscall is needed here since this test only uses the hit reference id for lookup purposes.server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalTopHitsTests.java (1)
155-159: LGTM!The test correctly adapts to the new
SearchHitAPI pattern. The two-step construction (creating withdocIdandid, then attaching fields viaaddDocumentFields) aligns with the PR's refactoring to simplify theSearchHitconstructor. The emptysearchHitFieldsmap is appropriate here since this test focuses on TopHits aggregation scoring and sorting behavior rather than document field handling.x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/execution/TriggeredWatchStoreTests.java (2)
228-231: LGTM!The
SearchHitconstructor update correctly uses the new simplified two-argument signature, removing the trailing null maps. The subsequent setup for version, shard target, and source reference remains unchanged, preserving the test's intent.
243-246: LGTM!Consistent with the first change—correctly adapts to the simplified
SearchHitconstructor while maintaining the same test setup pattern for the second search hit used in scroll pagination testing.server/src/main/java/org/elasticsearch/index/fieldvisitor/CustomFieldsVisitor.java (1)
17-18: LGTM!The updated class documentation accurately reflects that
_idand_routingare always loaded by the parentFieldsVisitor, clarifying the behavior for future maintainers.server/src/main/java/org/elasticsearch/index/mapper/LegacyTypeFieldMapper.java (2)
32-41: Centralized FIELD_TYPE usage looks good.
The shared constant and constructor update align with the refactor and reduce redundant instances.
49-49: Encapsulating the field type class is a nice cleanup.
Limiting visibility here keeps the API surface tighter without affecting behavior.server/src/main/java/org/elasticsearch/index/mapper/IgnoredFieldMapper.java (1)
43-76: FIELD_TYPE exposure and reuse look consistent.
This aligns with the refactor pattern and keeps construction clean.server/src/main/java/org/elasticsearch/index/mapper/RoutingFieldMapper.java (1)
70-104: Routing FIELD_TYPE constant and constructor change look good.
Consistent with the new public access pattern and avoids inline instantiation.server/src/test/java/org/elasticsearch/index/mapper/RoutingFieldTypeTests.java (3)
21-21: Test update matches the new FIELD_TYPE API.
38-38: Test update matches the new FIELD_TYPE API.
51-51: Test update matches the new FIELD_TYPE API.server/src/test/java/org/elasticsearch/index/mapper/IgnoredFieldTypeTests.java (3)
22-22: Test update matches the new FIELD_TYPE API.
39-39: Test update matches the new FIELD_TYPE API.
52-52: Test update matches the new FIELD_TYPE API.x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/extractor/TopHitsAggExtractorTests.java (1)
119-127: LGTM — new SearchHit construction is clear and consistent.server/src/main/java/org/elasticsearch/search/internal/SubSearchContext.java (1)
92-94: LGTM! More accurate semantic check for script fields presence.This change correctly distinguishes between having a
ScriptFieldsContextcontainer and actually having script fields defined. ThescriptFields.fields()method always returns a non-nullArrayList, so no NPE risk.Minor style nit:
!scriptFields.fields().isEmpty()is slightly more idiomatic thanscriptFields.fields().isEmpty() == false, but both are correct.server/src/main/java/org/elasticsearch/index/get/ShardGetService.java (2)
24-24: Import addition looks fine.Line 24 adds
MappedFieldTypeto support the new conversion logic.
270-274: Confirm behavior for stored fields with no current mapping.On Line 270–Line 274, fields are skipped when
mapperService.fieldType(...)returns null. If stored fields exist but mappings were removed/changed, this now omits requested fields instead of returning raw stored values. Please confirm this is intended (and covered by tests). If not, consider falling back to raw values whenftis null.Optional fallback if you want to preserve raw stored values
- MappedFieldType ft = mapperService.fieldType(entry.getKey()); - if (ft == null) { - continue; // user asked for a non-existent field, ignore it - } - List<Object> values = entry.getValue().stream().map(ft::valueForDisplay).toList(); + MappedFieldType ft = mapperService.fieldType(entry.getKey()); + List<Object> values = entry.getValue(); + if (ft != null) { + values = values.stream().map(ft::valueForDisplay).toList(); + }server/src/main/java/org/elasticsearch/search/SearchModule.java (1)
219-219: LGTM!The
StoredFieldsPhaseimport and registration follow the established pattern for fetch sub-phases. The placement in the registration order (afterExplainPhase) is consistent with the new stored fields handling architecture introduced in this PR.Also applies to: 993-993
x-pack/plugin/async-search/src/test/java/org/elasticsearch/xpack/search/AsyncSearchSingleNodeTests.java (1)
126-129: LGTM!The
storedFieldsSpec()override correctly returnsStoredFieldsSpec.NO_REQUIREMENTS, which is appropriate for this test stub since the processor only checks the hit ID and doesn't require any stored fields to be loaded.x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/action/TransportDeleteForecastActionTests.java (1)
88-90: LGTM!The refactored
SearchHitconstruction correctly uses the simplified two-argument constructor followed byaddDocumentFields()to attach the forecast status fields. This aligns with the PR's API simplification pattern.x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/action/EnrichShardMultiSearchAction.java (1)
272-274: LGTM!The simplified
SearchHitconstructor is appropriate here since the enrich action only needs to set the source viasourceRef()and doesn't require document fields or metadata fields. The previous empty map arguments were unnecessary.x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/WatcherServiceTests.java (1)
211-214: LGTM!The simplified
SearchHitconstructor is correctly used here. The test only requires the doc ID and hit ID for creating mock hits that are subsequently processed by the watch parser, which operates on source content rather than document fields.modules/parent-join/build.gradle (1)
23-25: LGTM!The test skip is necessary for V7 compatibility testing since the introduction of
StoredFieldsPhasechanges the profile output structure. The skip reason clearly documents why this test is excluded.rest-api-spec/build.gradle (1)
78-80: Skip entries align with profile output updates.
Rationale is clear and scoped to compatibility transforms.x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/saml/TransportSamlInvalidateSessionActionTests.java (1)
303-304: Updated SearchHit construction matches new API.
Cleaner instantiation without null placeholders.server/src/main/java/org/elasticsearch/search/fetch/subphase/ScriptFieldsContext.java (1)
42-42: Good immutability tightening forfields.
Prevents reassignment while preserving behavior.docs/reference/search/profile.asciidoc (1)
196-206: Docs now reflect StoredFieldsPhase profiling.
Examples stay consistent with updated fetch profiling output.Also applies to: 1025-1035
rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/370_profile.yml (3)
25-47: Profile test updated for StoredFieldsPhase in fetch fields.
Skip range and child expectations match new profile output.
56-82: Profile test updated for StoredFieldsPhase in fetch source.
Child count and phase type assertions are consistent.
86-150: Profile test updated for StoredFieldsPhase in nested fetch.
Additional child expectations reflect new phase ordering.modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorMatchedSlotSubFetchPhase.java (1)
29-30: Explicit stored-fields requirements are clear.
NO_REQUIREMENTSis appropriate for this sub-phase.Also applies to: 73-76
modules/reindex/src/test/java/org/elasticsearch/reindex/AsyncBulkByScrollActionTests.java (1)
568-568: LGTM!The
SearchHitconstruction correctly migrates to the new 2-argument form with source attachment viasourceRef(), consistent with the PR's API refactoring pattern.x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/datafeed/extractor/scroll/ScrollDataExtractorTests.java (1)
543-544: LGTM!The migration to the 2-argument
SearchHitconstructor withaddDocumentFields()for field attachment is correct and maintains the test's functionality.server/src/main/java/org/elasticsearch/search/fetch/StoredFieldsSpec.java (1)
38-46: LGTM!The
merge()method correctly creates a newHashSetto avoid mutating the original sets and properly combines the specifications using logical OR for boolean flags and set union for required fields.server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchScorePhase.java (1)
45-48: LGTM!The
storedFieldsSpec()override correctly returnsNO_REQUIREMENTSsince score computation uses Lucene'sScorerand doesn't require any stored fields to be loaded.server/src/main/java/org/elasticsearch/search/fetch/subphase/ScriptFieldsPhase.java (1)
42-49: LGTM!The
storedFieldsSpec()correctly declares that script fields require metadata loading (for access to fields like_id,_routing) while deferring source loading toSearchLookup's lazy mechanism. The inline comment provides helpful context for this design decision.server/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/HighlightPhase.java (1)
19-27: StoredFieldsSpec plumbing is clear and well-contained.
Returning the precomputed spec from the processor keeps fetch orchestration aligned with highlighting needs, and reusing the prebuilt builders avoids per-hit recomputation.Also applies to: 49-66
server/src/main/java/org/elasticsearch/search/SearchHit.java (1)
88-152: Non-null field maps and simplified constructors look good.
Eager map initialization plus the streamlined constructors (andcreateFromMapusingaddDocumentFields) keep field handling consistent and avoid null checks.Also applies to: 843-845
x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java (1)
905-911: Test hit construction aligned with the new SearchHit API.
Using the(docId, id)constructor keeps this test consistent with the updated usage pattern.x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/dataframe/process/DataFrameRowsJoinerTests.java (1)
311-314: SearchHit construction update looks good.
Switching to the two-arg constructor and setting the source explicitly matches the new API pattern.x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/execution/search/CriterionOrdinalExtractionTests.java (1)
150-156: Field attachment viaaddDocumentFieldsis consistent.
The updated construction keeps the test data aligned with the newSearchHitAPI while preserving field values.modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorHighlightSubFetchPhase.java (2)
21-66: StoredFieldsSpec override is appropriate here.
Explicitly returningNO_REQUIREMENTSclarifies that percolator highlighting doesn’t rely on stored fields from the main fetch phase.
92-96: HitContext construction updated cleanly.
Using the newSearchHitconstructor and supplying the loaded-fields map keeps this sub-context aligned with the new signature.x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/TokenServiceTests.java (1)
1016-1016: LGTM: constructor update aligns with the new SearchHit API.x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractorTests.java (2)
96-99: LGTM: field attachment after SearchHit construction is consistent with the new pattern.Also applies to: 115-117, 130-132, 147-149, 166-168, 177-179, 190-192, 207-209, 214-216, 226-228, 240-242
157-158: No change needed. The 3-argSearchHitconstructor still exists and the code at line 157 compiles as-is. The constructor signatureSearchHit(int nestedTopDocId, String id, NestedIdentity nestedIdentity)is available in the current codebase.Likely an incorrect or invalid review comment.
server/src/internalClusterTest/java/org/elasticsearch/action/search/TransportSearchIT.java (1)
96-99: LGTM: explicit StoredFieldsSpec declaration is clear and minimal.server/src/test/java/org/elasticsearch/action/search/SearchPhaseControllerTests.java (1)
425-436: LGTM: simplified SearchHit construction matches updated API.x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/service/IndexServiceAccountTokenStoreTests.java (1)
266-269: LGTM: SearchHit construction updated cleanly.modules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/DiscountedCumulativeGainTests.java (1)
63-64: LGTM: constructor updates are consistent with the new SearchHit API.Also applies to: 113-114, 170-171
server/src/main/java/org/elasticsearch/search/fetch/subphase/MatchedQueriesPhase.java (1)
76-79: Stored fields requirement declaration looks good.
ReturningNO_REQUIREMENTSmatches this sub-phase’s behavior.server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchDocValuesPhase.java (1)
63-66: Stored fields requirement declaration looks good.
NO_REQUIREMENTSaligns with doc values fetching.server/src/main/java/org/elasticsearch/search/fetch/subphase/ExplainPhase.java (1)
47-50: Stored fields requirement declaration looks good.
NO_REQUIREMENTSis appropriate for explain.server/src/internalClusterTest/java/org/elasticsearch/search/fetch/FetchSubPhasePluginIT.java (1)
120-123: Stored fields requirement declaration looks good.
This test sub-phase correctly declares no stored fields.server/src/test/java/org/elasticsearch/search/fetch/subphase/FetchSourcePhaseTests.java (1)
173-179: Test updates align with the new constructors.
The updatedSearchHitandHitContextinstantiations look correct.modules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/RatedSearchHitTests.java (2)
29-29: Constructor update looks good.
This aligns with the simplifiedSearchHitAPI.
39-39: Constructor update looks good.
Mutation path uses the newSearchHitsignature consistently.server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchVersionPhase.java (1)
17-39: Good alignment with stored-fields contract.
ReturningStoredFieldsSpec.NO_REQUIREMENTShere is consistent with the phase’s needs and keeps behavior unchanged.modules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/MeanReciprocalRankTests.java (1)
190-194: Test update matches new SearchHit constructor.
This keeps the test aligned with the new SearchHit API.server/src/test/java/org/elasticsearch/action/search/SearchResponseTests.java (1)
217-219: SearchHit construction updated appropriately.
Using the two-arg constructor keeps the test in sync with the new API.modules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/RecallAtKTests.java (1)
101-107: Constructor changes look good.
The updated SearchHit instantiation matches the new API and keeps the tests consistent.Also applies to: 216-221
modules/percolator/src/test/java/org/elasticsearch/percolator/PercolatorMatchedSlotSubFetchPhaseTests.java (1)
36-38: HitContext updates are consistent with new signature.
PassingMap.of()matches the constructor change and keeps the test setup clean.Also applies to: 59-60, 90-91, 120-121
server/src/test/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhaseTests.java (1)
32-36: Test wiring matches updated HitContext signature.
The added empty map is appropriate for this test case.Also applies to: 84-87
modules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/RankEvalResponseTests.java (1)
199-203: LGTM: constructor update matches the new SearchHit API.modules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/ExpectedReciprocalRankTests.java (1)
100-108: LGTM: aligned SearchHit construction in tests.server/src/main/java/org/elasticsearch/search/fetch/FetchProfiler.java (1)
125-128: LGTM: profiling wrapper now exposes stored-fields requirements.server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.java (1)
11-18: No review needed for import-only change.server/src/main/java/org/elasticsearch/search/fetch/subphase/SeqNoPrimaryTermPhase.java (2)
14-18: No review needed for import-only change.
38-41: LGTM: stored-fields requirement accurately declared as none.server/src/main/java/org/elasticsearch/search/fetch/subphase/InnerHitsPhase.java (1)
46-56: LGTM! Clean implementation of storedFieldsSpec() override.The
StoredFieldsSpecis correctly computed once during processor creation and captured in the closure for efficient reuse. The implementation properly declares source requirements based on the inner hits' configurations.server/src/test/java/org/elasticsearch/search/SearchHitsTests.java (2)
229-229: LGTM! Test updated to use simplified SearchHit constructor.The test correctly uses the new two-argument constructor, aligning with the broader API simplification in this PR.
253-253: LGTM! Consistent with the SearchHit constructor simplification.server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchSourcePhase.java (2)
40-43: LGTM! Correct storedFieldsSpec() implementation.Returning
StoredFieldsSpec.NEEDS_SOURCEis the appropriate choice for FetchSourcePhase since it needs to load the document source.
47-47: LGTM! Index name retrieval scoped appropriately.Moving the index name retrieval into
process()keeps it close to its usage in the error message and avoids capturing it unnecessarily in the outer scope.server/src/main/java/org/elasticsearch/search/fetch/StoredFieldsContext.java (2)
34-34: LGTM! Good immutability improvement.Making
fetchFieldsfinal enforces immutability for this field, which is a sound practice.
146-148: Confirm thatmetadataOnly()contexts prevent field additions via null check.The
metadataOnly()factory creates a context withfieldNames = nullandfetchFields = true. CallingaddFieldName()oraddFieldNames()on such a context will throw aNullPointerExceptionat lines 99 and 88 respectively. This design pattern successfully prevents mutation of metadata-only contexts, though the safety mechanism is implicit rather than explicit (the null reference silently rejects modifications). The single usage in the codebase (FetchContext.java:84) confirms this is the intended pattern—contexts created bymetadataOnly()are used directly without further field additions.server/src/main/java/org/elasticsearch/search/fetch/FetchContext.java (4)
50-52: LGTM! Clean initialization of context fields.The constructor properly delegates to builder methods for both stored fields and fetch source contexts.
80-87: LGTM! Clean default handling for stored fields context.The method correctly defaults to
metadataOnly()when no stored fields context is provided.
107-107: LGTM! Simplified lookup retrieval.Getting the lookup directly from
SearchExecutionContextinstead of storing it as a field is cleaner and avoids potential staleness issues.
141-150: LGTM! Clean accessors for the new context fields.The accessors return the pre-computed contexts, consistent with the rest of the class design.
server/src/test/java/org/elasticsearch/action/search/ExpandSearchPhaseTests.java (5)
47-47: LGTM! Consistent use of simplified SearchHit constructor.
111-113: LGTM! Proper pattern for constructing SearchHit with document fields.The test correctly uses the two-argument constructor followed by
setDocumentField()to attach the collapse field, aligning with the PR's refactored approach.
140-140: LGTM!
178-182: LGTM! Consistent pattern applied to multiple hits.
206-210: LGTM! Test correctly updated for the new SearchHit construction pattern.server/src/main/java/org/elasticsearch/search/fetch/FetchSubPhase.java (1)
71-73: LGTM!The accessor method for
loadedFieldsis appropriately implemented, providing access to the stored fields map for fetch sub-phases to process.modules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/PrecisionAtKTests.java (2)
103-104: LGTM!The update to use the simplified two-argument
SearchHitconstructor is consistent with the API refactoring. Since this test only needs basic hit identification for rank evaluation, the new constructor is appropriate.
248-255: LGTM!The
toSearchHitshelper method is correctly updated to use the newSearchHit(docId, id)constructor. The test logic remains unchanged and properly validates precision-at-k calculations.server/src/main/java/org/elasticsearch/search/fetch/subphase/StoredFieldsPhase.java (3)
91-119: LGTM on the processor implementation.The anonymous
FetchSubPhaseProcessorimplementation is well-structured:
- Empty
setNextReaderis appropriate since no per-segment state is needed- The
processmethod correctly categorizes fields into document fields and metadata fieldsstoredFieldsSpec()properly returns the pre-computed spec
39-54: LGTM on the StoredField record.The inner record cleanly encapsulates field metadata and processing logic. Using
ft::valueForDisplayto transform stored values is the correct approach for consistent value representation.
70-89: I need the review comment to rewrite. Please provide it within<review_comment>tags along with any relevant repository context or file paths for verification.server/src/test/java/org/elasticsearch/search/fetch/StoredFieldsSpecTests.java (2)
26-46: LGTM on testDefaults.The test correctly validates:
- Default source loading behavior (
requiresSource=true)- Default stored fields behavior (
requiresSource=false)- Merge semantics preserve source requirement
48-55: LGTM on testStoredFieldsDisabled.Good test coverage verifying that
"_none_"disables both stored fields and source fetching, returningnullprocessors.server/src/test/java/org/elasticsearch/search/SearchHitTests.java (2)
74-75: LGTM!The test correctly adapts to the new
SearchHitconstructor pattern, constructing the hit first and then attaching fields viaaddDocumentFields.
228-246: LGTM!The test correctly creates nested inner hits using the simplified constructor, maintaining proper shard target propagation verification.
server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java (4)
97-106: LGTM on StoredFieldsSpec aggregation.The logic correctly:
- Starts with
NO_REQUIREMENTSas the base- Merges each processor's spec
- Includes the source loader's required stored fields
- Derives the
requiresSourceflag from the final merged specThis centralized approach ensures consistent stored field loading across all fetch sub-phases.
236-256: LGTM on non-nested hit context preparation.The logic correctly handles:
- Null ID case: passes empty
Map.of()for stored fields- Normal case: passes
leafStoredFieldLoader.storedFields()to HitContext- Source loading is conditional on
requiresSourceflag, improving performance when source isn't needed
350-358: LGTM on nested hit context preparation.The nested hit handling correctly:
- Passes
childFieldLoader.storedFields()to HitContext- Returns appropriate Source (from nested source map or empty) based on whether source was loaded
161-166: No issues found. ThenoRequirements()method is properly defined on theStoredFieldsSpecrecord atserver/src/main/java/org/elasticsearch/search/fetch/StoredFieldsSpec.java(lines 21-23) and correctly called in the code.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if (sfc != null && sfc.fetchFields()) { | ||
| for (String field : sfc.fieldNames()) { | ||
| if (SourceFieldMapper.NAME.equals(field)) { | ||
| fsc = fsc == null ? FetchSourceContext.of(true) : FetchSourceContext.of(true, fsc.includes(), fsc.excludes()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential NullPointerException when iterating over fieldNames.
When storedFieldsContext is created via metadataOnly(), fieldNames() returns null. The for-each loop at line 68 will throw an NPE in this case.
🐛 Proposed fix: Add null check before iteration
- if (sfc != null && sfc.fetchFields()) {
+ if (sfc != null && sfc.fetchFields() && sfc.fieldNames() != null) {
for (String field : sfc.fieldNames()) {
if (SourceFieldMapper.NAME.equals(field)) {
fsc = fsc == null ? FetchSourceContext.of(true) : FetchSourceContext.of(true, fsc.includes(), fsc.excludes());
}
}
}🤖 Prompt for AI Agents
In `@server/src/main/java/org/elasticsearch/search/fetch/FetchContext.java` around
lines 67 - 73, The loop over sfc.fieldNames() can NPE when fieldNames() returns
null (e.g., metadataOnly()); before iterating, guard with a null check on
sfc.fieldNames() — in the block that checks sfc != null && sfc.fetchFields(),
add a condition or early continue ensuring sfc.fieldNames() != null, then
perform the for-each and update fsc using FetchSourceContext.of(true) when
SourceFieldMapper.NAME matches.
| /** | ||
| * The stored fields or source required by this sub phase | ||
| */ | ||
| StoredFieldsSpec storedFieldsSpec(); |
There was a problem hiding this comment.
Potential binary compatibility break for plugin sub-phases.
Adding a new abstract method on this SPI will break existing plugins at runtime unless they are recompiled. If backward compatibility is desired, make it a default method that returns StoredFieldsSpec.NO_REQUIREMENTS.
🛠️ Suggested compatibility-preserving default
- StoredFieldsSpec storedFieldsSpec();
+ default StoredFieldsSpec storedFieldsSpec() {
+ return StoredFieldsSpec.NO_REQUIREMENTS;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * The stored fields or source required by this sub phase | |
| */ | |
| StoredFieldsSpec storedFieldsSpec(); | |
| /** | |
| * The stored fields or source required by this sub phase | |
| */ | |
| default StoredFieldsSpec storedFieldsSpec() { | |
| return StoredFieldsSpec.NO_REQUIREMENTS; | |
| } |
🤖 Prompt for AI Agents
In
`@server/src/main/java/org/elasticsearch/search/fetch/FetchSubPhaseProcessor.java`
around lines 40 - 43, The new abstract method storedFieldsSpec() on
FetchSubPhaseProcessor will break binary compatibility for existing plugins;
change its declaration to a default method on the FetchSubPhaseProcessor
interface that returns StoredFieldsSpec.NO_REQUIREMENTS so existing
implementations continue to work without recompilation, i.e., update the
storedFieldsSpec() signature in FetchSubPhaseProcessor to a default
implementation returning StoredFieldsSpec.NO_REQUIREMENTS.
| public void addDocumentFields(Map<String, DocumentField> docFields, Map<String, DocumentField> metaFields) { | ||
| this.documentFields.putAll(docFields); | ||
| this.metaFields.putAll(metaFields); | ||
| } |
There was a problem hiding this comment.
Guard addDocumentFields against null inputs.
This public method will throw if a caller passes null (previous constructors tolerated null maps). A small guard keeps the API safe.
🛡️ Null-safe merge
public void addDocumentFields(Map<String, DocumentField> docFields, Map<String, DocumentField> metaFields) {
- this.documentFields.putAll(docFields);
- this.metaFields.putAll(metaFields);
+ if (docFields != null) {
+ this.documentFields.putAll(docFields);
+ }
+ if (metaFields != null) {
+ this.metaFields.putAll(metaFields);
+ }
}🤖 Prompt for AI Agents
In `@server/src/main/java/org/elasticsearch/search/SearchHit.java` around lines
433 - 436, The public method SearchHit.addDocumentFields currently calls putAll
directly and will NPE if callers pass null; update addDocumentFields to guard
against null inputs by checking docFields and metaFields before calling
this.documentFields.putAll(docFields) and this.metaFields.putAll(metaFields)
(treat null as no-op/empty), so the method tolerates null maps and preserves
previous constructor behavior.
| Map<String, DocumentField> metadata = new HashMap<>(); | ||
| metadata.put("_routing", new DocumentField("_routing", Collections.emptyList())); | ||
| hit = new SearchHit(0, "_id", fields, Collections.emptyMap()); | ||
| hit = new SearchHit(0, "_id"); | ||
| hit.addDocumentFields(fields, Collections.emptyMap()); |
There was a problem hiding this comment.
Unused metadata map - test logic may be incomplete.
A metadata map is created with a _routing field (Line 397), but Line 399 passes Collections.emptyMap() instead of metadata to addDocumentFields. This appears to be either:
- A copy-paste error from the previous test block
- An incomplete test that intended to verify metadata field handling
If the intent is to test that empty metadata fields are omitted from serialization, the metadata map should be passed:
🐛 Proposed fix
Map<String, DocumentField> metadata = new HashMap<>();
metadata.put("_routing", new DocumentField("_routing", Collections.emptyList()));
hit = new SearchHit(0, "_id");
-hit.addDocumentFields(fields, Collections.emptyMap());
+hit.addDocumentFields(fields, metadata);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Map<String, DocumentField> metadata = new HashMap<>(); | |
| metadata.put("_routing", new DocumentField("_routing", Collections.emptyList())); | |
| hit = new SearchHit(0, "_id", fields, Collections.emptyMap()); | |
| hit = new SearchHit(0, "_id"); | |
| hit.addDocumentFields(fields, Collections.emptyMap()); | |
| Map<String, DocumentField> metadata = new HashMap<>(); | |
| metadata.put("_routing", new DocumentField("_routing", Collections.emptyList())); | |
| hit = new SearchHit(0, "_id"); | |
| hit.addDocumentFields(fields, metadata); |
🤖 Prompt for AI Agents
In `@server/src/test/java/org/elasticsearch/search/SearchHitTests.java` around
lines 396 - 399, The test creates a metadata Map<String, DocumentField>
containing a "_routing" DocumentField but then calls
SearchHit.addDocumentFields(fields, Collections.emptyMap()), leaving the
metadata unused; update the call to pass the created metadata map so the test
exercises metadata handling (i.e., replace the empty map argument with the
metadata variable) and verify SearchHit serialization/deserialization or
assertions that rely on the "_routing" metadata field (locate the SearchHit
instantiation, the metadata Map declaration, and the addDocumentFields(...)
call).
| SearchHit hit = new SearchHit(10, null); | ||
| hit.addDocumentFields(Map.of(), fields.get(i)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find SearchHit class and its addDocumentFields method
fd -t f 'SearchHit.java' --type fRepository: Signal65/elasticsearch-CodeRabbit
Length of output: 222
🏁 Script executed:
# Search for addDocumentFields method in SearchHit.java
rg -n -A 10 'void addDocumentFields' server/src/main/java/org/elasticsearch/search/SearchHit.javaRepository: Signal65/elasticsearch-CodeRabbit
Length of output: 522
Fix the argument order in addDocumentFields call.
The test fields should be passed as the first argument (document fields), not the second (metadata fields). Based on the addDocumentFields(Map<String, DocumentField> docFields, Map<String, DocumentField> metaFields) signature, the correct call should be:
hit.addDocumentFields(fields.get(i), Map.of());
🤖 Prompt for AI Agents
In
`@x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/persistence/MockClientBuilder.java`
around lines 192 - 193, The call to SearchHit.addDocumentFields has its
arguments reversed: pass the test document fields as the first argument and the
empty metadata map as the second; update the call in MockClientBuilder where
SearchHit hit = new SearchHit(10, null) is created to call addDocumentFields
with fields.get(i) as the first parameter and Map.of() as the second (i.e.,
correct the argument order for addDocumentFields(Map<String, DocumentField>
docFields, Map<String, DocumentField> metaFields)).
2 participants
PR_011
Summary by CodeRabbit
New Features
Refactors
✏️ Tip: You can customize this high-level summary in your review settings.